fix(web): allow to set default keyboard to 'off' - #16524
Conversation
keymanapp-test-bot
Bot
added
the
user-test-missing
User Test ResultsTest specification and instructions ERROR: user tests have not yet been defined Test Artifacts |
5d4a63b to
466b822
Compare
466b822 to
01921a2
Compare
01921a2 to
a21df46
Compare
96c527c to
9de509a
Compare
| } | ||
|
|
||
| test.describe.skip('First example from the guide', function () { | ||
| test.describe('First example from the guide', function () { |
There was a problem hiding this comment.
The guide-examples e2e tests are re-enabled; if it turns out that they're still not stable then we can disable them again.
9de509a to
f82c68d
Compare
|
|
||
| languageMenu.lgList.style.display='none'; //still allows blank menu momentarily on selection | ||
| languageMenu.keyman.contextManager.activateKeyboard(entry.kn, entry.kc,true); | ||
| languageMenu.keyman.contextManager.restoreLastActiveTextStore(); |
There was a problem hiding this comment.
This is already done in activateKeyboard, so there's no need to do it twice (especially since activateKeyboard is async, so this call could possibly work with outdated data...)
The parameters for `setKeyboardForControl` basically have three different modes: - an id of a keyboard/language to set that keyboard, enabling independent keyboard mode - empty string to set the system keyboard, enabling independent keyboard mode - `null` to disable independent keyboard mode. Our previous code didn't properly handle the last two modes. This change fixes the problems and also clarifies and updates the documentation. Also included is an improvement to the guide-examples e2e tests that now wait until all keyboards are loaded (which may fix #16167). Partially drafted by kilo.ai. Fixes: #16080 Build-bot: skip release:web
f82c68d to
4d7867e
Compare
| if (!elem.ownerDocument.defaultView) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
What role does this new conditional play?
| * @param langId | ||
| */ | ||
| public setKeyboardForTextStore(textStore: AbstractElementTextStore<any>, kbdId: string, langId: string): void { | ||
| public setKeyboardForTextStore(textStore: AbstractElementTextStore<any>, kbdId: string | null, langId: string | null): void { |
There was a problem hiding this comment.
Why not use kbdId?: string, langId?: string instead? That way, there's no need for the nullish fallbacksin line 317 of keymanEngine.ts.
| } | ||
|
|
||
| this.contextManager.setKeyboardForTextStore(elem._kmwAttachment.textStore, keyboard, languageCode); | ||
| this.contextManager.setKeyboardForTextStore(elem._kmwAttachment.textStore, keyboard ?? null, languageCode ?? null); |
There was a problem hiding this comment.
I would prefer changes that leave these two parameters completely pass-through. Why change this line when an equally-simple change avoids the need for it?
| constructInstance: (): null => null | ||
| }; | ||
|
|
||
| describe('KeymanEngine.getKeyboardForControl', () => { |
There was a problem hiding this comment.
No explicit .setKeyboardForControl tests here?
While I guess they're kinda tied, you should also be able to verify three things:
- A keystroke (either physical or via OSK) results in the correct output character
- Swapping to a second, still-global control swaps the current "active keyboard" reported by the engine to the global keyboard setting.
- Swapping back to the original control restores its setting and what the "current keyboard" reported by the engine is.
I thought we had some old automated tests that might have already been testing points 2 and 3, but I don't see them upon a search.
They did exist back in stable-16.0, but apparently they got erased at some point by accident during work toward stable-17.0. Here's a permalink to the relevant automated tests from before:
3 participants
The parameters for
setKeyboardForControlbasically have three different modes: - an id of a keyboard/language to set that keyboard, enabling independent keyboard mode - empty string to set the system keyboard, enabling independent keyboard mode -nullto disable independent keyboard mode.Our previous code didn't properly handle the last two modes. This change fixes the problems and also clarifies and updates the documentation.
Also included is an improvement to the guide-examples e2e tests that now wait until all keyboards are loaded (which may fix #16167).
Partially drafted by kilo.ai.
Fixes: #16080
Build-bot: skip release:web